-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixed textpoint alignment #6967
base: main
Are you sure you want to change the base?
Conversation
@@ -344,7 +344,8 @@ p5.Font = class { | |||
textToPoints(txt, x, y, fontSize, options) { | |||
const xOriginal = x; | |||
const result = []; | |||
|
|||
const p = this.parent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means that it will use the alignment of the main canvas even when one is using the font to draw points on a p5.Graphics
. That could be fine, since I'm not sure there's any better way to determine where you're going to use the points after you call this function. @dhowe what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means that it will use the alignment of the main canvas even when one is using the font to draw points on a
p5.Graphics
. That could be fine, since I'm not sure there's any better way to determine where you're going to use the points after you call this function. @dhowe what do you think?
I don't love this, but it does highlight a bigger problem with the class, specifically that it cannot be used outside of p5 at present, which is something I think (?) we want to support (see #6830). My initial thinking for this class was that it was font-specific but rendering-agnostic and thus could be used with/w'out p5.
So my feeling is that it should probably be refactored only to access the p5.renderer if parameters are not otherwise specified AND the renderer actually exists, otherwise it should use sensible defaults. Obviously not a big priority as its been like this for awhile, but would be nice for 2.0 (and perhaps integrated into #6830)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense to me. In the mean time, do you think this change is ok to merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking that perhaps we should include alignment as an option via the options
argument? For cases like drawing to an offscreen buffer with different alignment than the main canvas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhowe Should we include the above as a default option, or do you think the alignment should be a required argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for now it's okay to expect users to set textAlignment
in the main p5 canvas before calling textToPoints
. If we wanted to pass alignment as a parameter, we'd also have to pass the values from textLeading
, textWrap
, not sure if any others (maybe textStyle
?).
If it needs to be added, I'd rather move textSize
and and all the other text...
values to the options
object than keep adding additional parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the intention, to use the options argument, not to change the function signature, so yes, I guess I'd argue for supporting all the related parameters in that object. The other option would be to only support these via the main renderer options, but this a) creates some potentially awkward code when drawing to an offscreen buffer, and b) doesn't allow for the class to be used outside of p5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_handleAlignment
itself is written as expecting a renderer to calculate the alignment. I guess to me it just feels weird to allow the user to change the alignment without using the renderer, since the function TextBounds()
in the same class similarly uses the parent's renderer to calculate its own alignment. A solution could be to handle the alignment without using _handeAlignment
, but I'm not sure if that would be the best solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a key question is whether we want this class to be usable outside of p5, as discussed in #6830 (I would argue that we do, but that's just me)... If so there are easy answers to many of the questions raised above; if not, then we only need to think about cases when drawing to something other than the main canvas (likely not a very common use-case). @limzykenneth @Qianqianye ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think even the case of the text points not being used directly in the canvas but rather sent as data to a server that can do other things with it (eg. control 3D printer or other computational manufacturing process) should probably be considered still. The more independent we can make each module the better I think, regardless of whether we end up making it usable outside of p5/browser environment.
@@ -376,6 +377,14 @@ p5.Font = class { | |||
|
|||
for (let l = 0; l < pts.length; l++) { | |||
pts[l].x += xoff; | |||
pos = this._handleAlignment( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@mathewpan2 @dhowe @davepagurek Just want to check in, is this ready to merge? We can revisit this as part of 2.0 but want to get fixes in for 1.x where possible and not breaking changes. |
I think this is ok to merge for now, with a note that this entire class (p5.Font) needs to be rethought for v2.0, where, assuming these function are not moved to a separate module or library, we should support use with and without the renderer in p5, and also, ideally, outside of p5. |
Resolves #6893
Changes:
Added the
_handleAlignment
function inp5.Font.js
totextToPoints
, so that it also follows the renderer's current alignment.Screenshots of the change:
When
textAlign(CENTER, BASELINE);
was called, the text points and the normal text would be misaligned.Before the change:
![error](https://private-user-images.githubusercontent.com/112679001/322154433-f9e443be-57c6-43c1-8f34-69c2f1fa882e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU3MTA0NTksIm5iZiI6MTcyNTcxMDE1OSwicGF0aCI6Ii8xMTI2NzkwMDEvMzIyMTU0NDMzLWY5ZTQ0M2JlLTU3YzYtNDNjMS04ZjM0LTY5YzJmMWZhODgyZS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwOTA3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDkwN1QxMTU1NTlaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT0xMWFlNjIxYWIzODljNTE1MGYzZjdiZmRkOWRhYzliMGJjNGY3ODIwNTA1ZTUzMmM3MTQ0MWMzNTc5M2NlYTBhJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.Lds8CZ1KY6-s73vth7EPLFdNBd-GGZmhLwa9BPAsjGM)
After the change:
![fixed](https://private-user-images.githubusercontent.com/112679001/322154567-3d8d0ee5-b553-461d-8067-926cf2c298fe.PNG?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU3MTA0NTksIm5iZiI6MTcyNTcxMDE1OSwicGF0aCI6Ii8xMTI2NzkwMDEvMzIyMTU0NTY3LTNkOGQwZWU1LWI1NTMtNDYxZC04MDY3LTkyNmNmMmMyOThmZS5QTkc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQwOTA3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MDkwN1QxMTU1NTlaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1mNjZmZDI2NGI1YWFjNjg5MmNiMWQxYTVjN2Y4YWEyYzg1YjAyNjVkZTNhZDI5ODg0YTgwZWY2ZTE0ODJiMzdhJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCZhY3Rvcl9pZD0wJmtleV9pZD0wJnJlcG9faWQ9MCJ9.qAk9ra9EuRxAvqFy9bg6DTXMpKpuXQsKwoTa2qoypSg)
PR Checklist
npm run lint
passes